CNF-14679: Adapt to single H/W manager plugin #220

tliu2021 · 2024-09-26T14:50:07Z

This update introduces the following changes:

Added an environment variable HWMGR_PLUGIN_NAMESPACE to the deployment to specify the namespace for the single plugin.
Introduced a new field hwMgrId in the NodePool spec to identify the plugin adapter.
Added ObservedGeneration fields for both the cloud manager and the hardware manager plugin in the NodePool status.

Note: To continue using the test plugin, use the following commands:
make run HWMGR_PLUGIN_NAMESPACE=oran-hwmgr-plugin-test
make deploy HWMGR_PLUGIN_NAMESPACE=oran-hwmgr-plugin-test

openshift-ci-robot · 2024-09-26T14:50:11Z

@tliu2021: This pull request references CNF-14679 which is a valid jira issue.

In response to this:

This update introduces the following changes:

Added an environment variable HWMGR_PLUGIN_NAMESPACE to the deployment to specify the namespace for the single plugin.

Introduced a new field hwMgrId in the NodePool spec to identify the plugin adapter.

Added ObservedGeneration fields for both the cloud manager and the hardware manager plugin in the NodePool status.

Note: To continue using the test plugin, use the following commands:
make run HWMGR_PLUGIN_NAMESPACE=oran-hwmgr-plugin-test
make deploy HWMGR_PLUGIN_NAMESPACE=oran-hwmgr-plugin-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

donpenney · 2024-09-26T17:40:23Z

Makefile

@@ -170,7 +173,7 @@ uninstall: manifests kustomize ## Uninstall CRDs from the K8s cluster specified
 .PHONY: deploy
 deploy: manifests kustomize ## Deploy controller to the K8s cluster specified in ~/.kube/config.
 	cd config/manager && $(KUSTOMIZE) edit set image controller=${IMG}
-	$(KUSTOMIZE) build config/default | $(KUBECTL) apply -f -
+	$(KUSTOMIZE) build config/default | sed 's|\plugin-namespace-placeholder|$(HWMGR_PLUGIN_NAMESPACE)|g' | $(KUBECTL) apply -f -


Is this a common method for doing such customizations? TALM sets a number of env variables by creating a patch.yaml that gets applied by the kustomization.yaml:
https://github.com/openshift-kni/cluster-group-upgrades-operator/blob/main/Makefile#L268

I am not sure if the TALM approach is the best option. I'll investigate further to see if there's a better way to handle this.

Kustomize in FluxCD (kustomize.toolkit.fluxcd.io) supports post-build variable substitution, but I don't think it's necessary to introduce that feature just for this. Instead, I'm using a ConfigMap to pass the environment variable to the deployment.

It just seemed like TALM's method of creating a patch with env vars is a little more formal than doing a sed to modify the output of the kustomize build while piping to the apply

I agree, but I believe using a ConfigMap with Kustomize Replacements is also a solid solution for this use case.

donpenney · 2024-09-26T17:44:33Z

internal/controllers/utils/utils.go

+// GetHwMgrPluginNS returns the value of environment variable HWMGR_PLUGIN_NAMESPACE
+func GetHwMgrPluginNS() string {
+	// Ensure that this code only runs once
+	once.Do(func() {


This once.Do is new to me... Very cool!

donpenney · 2024-09-26T18:23:02Z

api/hardwaremanagement/v1alpha1/node_pools.go

@@ -42,6 +42,9 @@ type NodePoolSpec struct {
 	//+operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Location Spec",xDescriptors={"urn:alm:descriptor:com.tectonic.ui:text"}
 	LocationSpec `json:",inline"`

+	// HwMgrId is the identifier for the hardware manager plugin adaptor.
+	HwMgrId string `json:"hwMgrId" yaml:"hwMgrId"`


This breaks the current plugin code, as it is adding a new required field. I think add omitempty here may avoid that. We can likely drop the omitempty later once the plugin is updated to know about the new field.

Verified that adding omitempty allows the current plugin code to work, which is referencing the previous version of the CRD, which it causes a failure to update the CR without it, as it would be missing this new field.

Suggested change

HwMgrId string `json:"hwMgrId" yaml:"hwMgrId"`

HwMgrId string `json:"hwMgrId,omitempty"`

Right, need to support the current plugin code.

donpenney · 2024-09-26T18:27:06Z

api/hardwaremanagement/v1alpha1/node_pools.go

@@ -42,6 +42,9 @@ type NodePoolSpec struct {
 	//+operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Location Spec",xDescriptors={"urn:alm:descriptor:com.tectonic.ui:text"}
 	LocationSpec `json:",inline"`

+	// HwMgrId is the identifier for the hardware manager plugin adaptor.
+	HwMgrId string `json:"hwMgrId" yaml:"hwMgrId"`


Is the yaml field necessary? The code seems inconsistent about using it. It's there for some fields, but not all. If it's not needed, if the yaml would default to the json field, we should drop it altogether

The yaml field is not needed and it will be removed.

donpenney · 2024-09-27T18:46:13Z

/retest

donpenney · 2024-09-27T18:47:53Z

/lgtm

donpenney · 2024-09-27T19:21:47Z

/retest

donpenney · 2024-10-01T12:41:40Z

Makefile

+		if [ "$(OS)" = "darwin" ]; then \
+			curl -LO "https://dl.k8s.io/release/$$(curl -L -s https://dl.k8s.io/release/stable.txt)/bin/darwin/$(ARCH)/kubectl"; \
+		else \
+			curl -LO "https://dl.k8s.io/release/$$(curl -L -s https://dl.k8s.io/release/stable.txt)/bin/linux/$(ARCH)/kubectl"; \


We should probably use a fixed version here and just always use kubectl from LOCALBIN. The Makefile has the ENVTEST_K8S_VERSION version from the original operator-sdk setup, I believe, but the envtest bits have been removed. It might be a bit nicer codewise to use it, actually, as it will handle the arch for us. You can see an example in the plugin Makefile:
https://github.com/openshift-kni/oran-hwmgr-plugin/blob/main/Makefile#L120

It downloads the latest setup-envtest binary with the envtest target, which is then used to download kubectl and return its path.

For example:

dpenney ~/testing/envtest $ mkdir bin dpenney ~/testing/envtest $ LOCALBIN=$PWD/bin dpenney ~/testing/envtest $ GOBIN=$LOCALBIN go install sigs.k8s.io/controller-runtime/tools/setup-envtest@latest go: downloading sigs.k8s.io/controller-runtime/tools/setup-envtest v0.0.0-20240927101401-4381fa0aeee4 dpenney ~/testing/envtest $ find bin bin bin/setup-envtest dpenney ~/testing/envtest $ bin/setup-envtest use 1.28.3 --bin-dir $LOCALBIN -p path /home/dpenney/testing/envtest/bin/k8s/1.28.3-linux-amd64 dpenney ~/testing/envtest $ find bin bin bin/setup-envtest bin/k8s bin/k8s/1.28.3-linux-amd64 bin/k8s/1.28.3-linux-amd64/etcd bin/k8s/1.28.3-linux-amd64/kube-apiserver bin/k8s/1.28.3-linux-amd64/kubectl

The Makefile has been updated.

Makefile

donpenney · 2024-10-01T12:59:43Z

/retest

Makefile

Signed-off-by: Tao Liu <tali@redhat.com>

alegacy · 2024-10-01T19:08:51Z

/lgtm

openshift-ci · 2024-10-01T21:26:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: donpenney

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [donpenney]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot requested review from browsell and jhernand September 26, 2024 14:50

donpenney requested review from alegacy, donpenney and rauhersu September 26, 2024 15:31

donpenney reviewed Sep 26, 2024

View reviewed changes

tliu2021 force-pushed the hwMgrId branch from 7c5776c to 95b09fe Compare September 27, 2024 17:59

openshift-ci bot assigned donpenney Sep 27, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2024

donpenney removed the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2024

tliu2021 force-pushed the hwMgrId branch from 95b09fe to 7a34823 Compare September 30, 2024 19:58

donpenney reviewed Oct 1, 2024

View reviewed changes

tliu2021 force-pushed the hwMgrId branch from 7a34823 to e7488e2 Compare October 1, 2024 14:27

donpenney reviewed Oct 1, 2024

View reviewed changes

Makefile Outdated Show resolved Hide resolved

CNF-14679: Adapt to single H/W manager plugin

0a2f99f

Signed-off-by: Tao Liu <tali@redhat.com>

tliu2021 force-pushed the hwMgrId branch from e7488e2 to 0a2f99f Compare October 1, 2024 16:41

openshift-ci bot assigned alegacy Oct 1, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 1, 2024

donpenney approved these changes Oct 1, 2024

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 1, 2024

openshift-merge-bot bot merged commit 0729dc2 into openshift-kni:main Oct 1, 2024
8 checks passed

tliu2021 deleted the hwMgrId branch November 5, 2024 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CNF-14679: Adapt to single H/W manager plugin #220

CNF-14679: Adapt to single H/W manager plugin #220

tliu2021 commented Sep 26, 2024

openshift-ci-robot commented Sep 26, 2024 •

edited by openshift-ci bot

Loading

donpenney Sep 26, 2024

tliu2021 Sep 26, 2024

tliu2021 Sep 27, 2024

donpenney Sep 27, 2024

tliu2021 Sep 27, 2024

donpenney Sep 26, 2024

donpenney Sep 26, 2024

donpenney Sep 26, 2024

tliu2021 Sep 26, 2024

tliu2021 Sep 27, 2024

donpenney Sep 26, 2024

tliu2021 Sep 27, 2024

donpenney commented Sep 27, 2024

donpenney commented Sep 27, 2024

donpenney commented Sep 27, 2024

donpenney Oct 1, 2024

tliu2021 Oct 1, 2024 •

edited

Loading

donpenney commented Oct 1, 2024

alegacy commented Oct 1, 2024

openshift-ci bot commented Oct 1, 2024

	HwMgrId string `json:"hwMgrId" yaml:"hwMgrId"`
	HwMgrId string `json:"hwMgrId,omitempty"`

CNF-14679: Adapt to single H/W manager plugin #220

CNF-14679: Adapt to single H/W manager plugin #220

Conversation

tliu2021 commented Sep 26, 2024

openshift-ci-robot commented Sep 26, 2024 • edited by openshift-ci bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donpenney commented Sep 27, 2024

donpenney commented Sep 27, 2024

donpenney commented Sep 27, 2024

Choose a reason for hiding this comment

tliu2021 Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

donpenney commented Oct 1, 2024

alegacy commented Oct 1, 2024

openshift-ci bot commented Oct 1, 2024

openshift-ci-robot commented Sep 26, 2024 •

edited by openshift-ci bot

Loading

tliu2021 Oct 1, 2024 •

edited

Loading